Parser!! #20
Parser!! #20
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bialger
left a comment
bialger
left a comment
There was a problem hiding this comment.
All in all, titanic work has been done. It is fantastic, and I don't see any major architecture flaws. Although there is a place to grow and some simple mistakes. Please fix them. Really good job, thank you.
…ing [[nodiscard]] attributes
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const Token& VectorTokenStream::Peek(size_t k) { | ||
| const Token* token = TryPeek(k); | ||
|
|
||
| if (token != nullptr) { | ||
| return *token; | ||
| } | ||
|
|
||
| if (last_ != nullptr) { | ||
| return *last_; | ||
| } | ||
|
|
||
| if (!tokens_.empty()) { | ||
| return *tokens_.back(); | ||
| } | ||
|
|
||
| throw std::out_of_range("VectorTokenStream::Peek out of range"); | ||
| } | ||
|
|
||
| TokenPtr VectorTokenStream::Consume() { | ||
| if (index_ < tokens_.size()) { | ||
| last_ = tokens_[index_].get(); | ||
| return std::move(tokens_[index_++]); | ||
| } | ||
|
|
||
| last_ = nullptr; |
There was a problem hiding this comment.
In VectorTokenStream::Consume the stream stores a raw pointer to the consumed token (last_ = tokens_[index_].get();) and then moves the unique_ptr out to the caller. Peek and LastConsumed later return *last_ when no tokens remain. If the caller releases the returned TokenPtr—which destroys the token—the stored pointer dangles and the next call to Peek or LastConsumed dereferences freed memory. The stream should either retain ownership of consumed tokens or avoid returning pointers/references after handing ownership to the caller; otherwise calls past EOF have undefined behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a comprehensive parser infrastructure for the Ovum compiler. It implements a type system with type references, nullability tracking, AST nodes for expressions/statements/declarations, visitor patterns for AST traversal, parser states, diagnostics with severity levels, and Pratt parsing infrastructure.
- Introduces
TypeReferenceclass with qualified names, type arguments, nullability, and resolution tracking - Implements full AST node hierarchy including expressions, statements, declarations, and class members
- Adds visitor pattern support with WalkVisitor, ConstWalkVisitor, PrintVisitor, StructuralValidator, and LintVisitor
- Provides diagnostic system with severity levels, error collection, and filtering capabilities
- Sets up parser state machine infrastructure and token stream handling
Reviewed Changes
Copilot reviewed 231 out of 231 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/parser/types/TypeReference.{hpp,cpp} | Core type reference implementation with qualified names, type arguments, nullability, and resolution |
| lib/parser/types/Param.{hpp,cpp} | Parameter representation with name and type |
| lib/parser/types/Nullable.{hpp,cpp} | Nullable type wrapper |
| lib/parser/ast/nodes/* | Complete AST node hierarchy for expressions, statements, declarations, and class members |
| lib/parser/ast/visitors/* | Visitor pattern implementations for AST traversal and validation |
| lib/parser/diagnostics/* | Diagnostic system with severity levels, collection, and filtering |
| lib/parser/states/* | Parser state machine infrastructure (headers only, implementations pending) |
| lib/parser/pratt/* | Pratt parser infrastructure for expression parsing |
| lib/parser/tokens/* | Token stream and source tracking utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [[nodiscard]] std::string GetName(); | ||
|
|
||
| [[nodiscard]] TypeReference GetType(); |
There was a problem hiding this comment.
The GetName() and GetType() methods should return const references for consistency with the rest of the codebase. These methods copy the entire std::string and TypeReference objects on every call, which is inefficient.
Change:
[[nodiscard]] std::string GetName();
[[nodiscard]] TypeReference GetType();To:
[[nodiscard]] const std::string& GetName() const;
[[nodiscard]] const TypeReference& GetType() const;
bialger
left a comment
bialger
left a comment
There was a problem hiding this comment.
Some of the reviewed changes. In general that looks good, but there is a place for improvement
| public: | ||
| virtual ~AstNode() = default; | ||
|
|
||
| // TODO: add positions...... |
| const TypeReference* ReturnType() const noexcept; | ||
| TypeReference* MutableReturnType() noexcept; |
There was a problem hiding this comment.
It would be much more safe to use a link here, raw pointers are undesirable in this context.
| const Block* Body() const noexcept; | ||
| Block* MutableBody() noexcept; |
There was a problem hiding this comment.
Same here and for all pointer-returning methods: use reference
| bool MethodDecl::IsStatic() const noexcept { | ||
| return is_static_; | ||
| } | ||
|
|
||
| void MethodDecl::SetStatic(bool is_static) noexcept { | ||
| is_static_ = is_static; | ||
| } |
There was a problem hiding this comment.
I am not sure whether we actually have static methods.
| std::string name; | ||
| std::vector<Param> params; | ||
| std::unique_ptr<TypeReference> ret_type; | ||
| std::unique_ptr<Block> body; // optional | ||
| }; |
There was a problem hiding this comment.
Non-public fields naming: should end with an underscore (e.g. name_)
| bool IsPublic() const noexcept; | ||
| void SetPublic(bool is_public) noexcept; | ||
|
|
||
| bool IsOverride() const noexcept; | ||
| void SetOverride(bool is_override) noexcept; | ||
|
|
||
| bool IsStatic() const noexcept; | ||
| void SetStatic(bool is_static) noexcept; | ||
|
|
||
| bool IsPure() const noexcept; | ||
| void SetPure(bool is_pure) noexcept; |
There was a problem hiding this comment.
I can't see why there and in other classes can't be a constructor for at least non-AstNode class members?
| void SetValue(bool new_value) noexcept; | ||
|
|
||
| private: | ||
| bool value = false; |
There was a problem hiding this comment.
Yet again, style for private fields: value_
| char Value() const noexcept; | ||
| void SetValue(char new_value) noexcept; |
There was a problem hiding this comment.
I am quite sure that in values you can do it with a constructor
No description provided.